VPC: Extend support for SG's #1989

cjschaef · 2024-10-03T21:43:04Z

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

/area provider/ibmcloud

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

Add reconciliation support for extended VPC Security Groups

k8s-ci-robot · 2024-10-03T21:43:09Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: cjschaef
Once this PR has been reviewed and has the lgtm label, please assign prajyot-parab for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2024-10-03T21:43:13Z

Hi @cjschaef. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

netlify · 2024-10-03T21:43:32Z

✅ Deploy Preview for kubernetes-sigs-cluster-api-ibmcloud ready!

Name	Link
🔨 Latest commit	`a2baa03`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-ibmcloud/deploys/67082973c48c370008facd33
😎 Deploy Preview	https://deploy-preview-1989--kubernetes-sigs-cluster-api-ibmcloud.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

mkumatag · 2024-10-07T09:53:46Z

/ok-to-test

Karthik-K-N · 2024-10-08T10:28:18Z

@dharaneeshvrd please take a look when get some time. Thank you.

Karthik-K-N

First pass, SG workflow looks bit complicated

Karthik-K-N · 2024-10-09T12:50:41Z

cloud/scope/vpc_cluster.go

+	// If we don't have an ID at this point, we assume we need to create the Security Group.
+	vpcID, err := s.GetVPCID()
+	if err != nil {
+		return false, fmt.Errorf("error retrieving vpc id for security group creation")


I think its better to log the err.

oops, yes I added the original err to the return value.

Karthik-K-N · 2024-10-09T12:52:06Z

cloud/scope/vpc_cluster.go

+		return false, fmt.Errorf("error retrieving vpc id for security group creation")
+	}
+	resourceGroupID, err := s.GetResourceGroupID()
+	if err != nil {


same, added the original error.

Karthik-K-N · 2024-10-09T12:53:27Z

cloud/scope/vpc_cluster.go

+		return false, fmt.Errorf("error failed to create security group: %w", err)
+	}
+	if securityGroupDetails == nil {
+		s.V(3).Info("error failed creating security group", "name", securityGroup.Name)


In general while logging, Instead of using generic term like "name", If we log key as "securitygroupname" or somthing, In the observability tools it will be easy to filter and debug.

ah, sorry, missed that. updating the logs that contain generic var names.

Karthik-K-N · 2024-10-09T12:54:22Z

cloud/scope/vpc_cluster.go

+	if securityGroupDetails == nil {
+		s.V(3).Info("error failed creating security group", "name", securityGroup.Name)
+		return false, fmt.Errorf("error failed creating security group")
+	} else if securityGroupDetails.ID == nil {


I know its defensive check, But is there any case happening like this ID or CRN being nil with no errors from create call?

Same situation, seems like a safer method that resulting in go panics.
I'll remove the additional checks of pointer fields.

Karthik-K-N · 2024-10-09T12:56:19Z

cloud/scope/vpc_cluster.go

+	// Add a tag to the Security Group for the cluster.
+	err = s.TagResource(s.IBMVPCCluster.Name, *securityGroupDetails.CRN)
+	if err != nil {
+		return false, fmt.Errorf("error failed to tag security group %s: %w", *securityGroupDetails.CRN, err)


Just a thought, if tagging fails here, In next reconcile we find id in status and we return, so that resource never being tagged.
May be need to revisit how better we can tag a resource.

Yes, the same situation occurs for the other resources. I will add the same comment here, that likely a found resource may need to be checked if it should be tagged.....knowing that existing resources may not have been created by CAPI, and thus should not be tagged.

Karthik-K-N · 2024-10-09T12:59:52Z

cloud/scope/vpc_cluster.go

+	var securityGroupID *string
+	if securityGroup.Name != nil {
+		securityGroupID = s.getSecurityGroupIDFromStatus(*securityGroup.Name)
+	} else if securityGroup.ID != nil {


If ID is set why can't we directly use it, Why do you think we need to iterate over status? If ID is not found in Cloud, In reconcileSecurityGroup func we are returning error.

I'm unsure why this safety check is bad, as we attempt to confirm the Security Group exists (ID is valid).
At this point with the code path, this is very unlikely, but it is a safety mechanism.

I will remove, but feel like these kind of checks are safer for future changes.

ok, My point is anyhow we are checking for id in previous step.

Karthik-K-N · 2024-10-09T13:01:14Z

cloud/scope/vpc_cluster.go

+	}
+
+	// If the SecurityGroup has no rules, we have nothing more to do for this Security Group.
+	if len(securityGroup.Rules) == 0 {


Can we move this check first thing in this function so we can return early

I will update.

Karthik-K-N · 2024-10-09T14:35:20Z

cloud/scope/vpc_cluster.go

+	}
+
+	// If the Security Group has no Rules at all, we simply create all the Rules
+	if existingSecurityGroupRuleIntfs == nil || existingSecurityGroupRuleIntfs.Rules == nil || len(existingSecurityGroupRuleIntfs.Rules) == 0 {


is existingSecurityGroupRuleIntfs.Rules == nil required? anyhow we are checking len()?

I will update.

Karthik-K-N · 2024-10-09T14:36:45Z

cloud/scope/vpc_cluster.go

+		if err != nil {
+			return false, fmt.Errorf("error failed creating all security group rule remotes: %w", err)
+		}
+		s.V(3).Info("Created security group rules")


Since its in V(3) level can we include some more info like, security group id or rule?

I will update.

Add support to reconcile SecurityGroups and SecurityGroupRules for VPC extended Infrastructure support. Related: kubernetes-sigs#1896

cjschaef · 2024-10-10T19:22:53Z

Updates made, basic testing appears to be okay.

Karthik-K-N

Few smaller things, Can be ignored if no other comments from others. Thank you. LGTM.

Karthik-K-N · 2024-10-14T08:00:03Z

cloud/scope/vpc_cluster.go

+
+		// Option #1: If the SecurityGroupRuleRemoteSecurityGroupReference has a name assigned, we can shortcut and simply check that
+		if sgRule.Name != nil && *securityGroupRuleRemote.SecurityGroupName == *sgRule.Name {
+			s.V(3).Info("security group rule remote security group name matches", "securityGroupRuleRemoteSecurityGroupName", sgRule.Name)


Should this be?

Suggested change

s.V(3).Info("security group rule remote security group name matches", "securityGroupRuleRemoteSecurityGroupName", sgRule.Name)

s.V(3).Info("security group rule remote security group name matches", "securityGroupRuleRemoteSecurityGroupName", *sgRule.Name)

Will update

Karthik-K-N · 2024-10-14T08:00:59Z

cloud/scope/vpc_cluster.go

+		// Option #3. We can compare the Security Group CRN (need ot lookup the CRN for securityGroupRemote)
+
+		// Option #1: If the SecurityGroupRuleRemoteSecurityGroupReference has a name assigned, we can shortcut and simply check that
+		if sgRule.Name != nil && *securityGroupRuleRemote.SecurityGroupName == *sgRule.Name {


Since your checking for sgRule.Name for nil, may be starting second condition from it would be good.

Suggested change

if sgRule.Name != nil && *securityGroupRuleRemote.SecurityGroupName == *sgRule.Name {

if sgRule.Name != nil && *sgRule.Name == *securityGroupRuleRemote.SecurityGroupName{

Will update

Karthik-K-N · 2024-10-14T08:02:32Z

cloud/scope/vpc_cluster.go

+			return true, nil
+		}
+		if *ipRule.Address == infrav1beta2.CIDRBlockAny {
+			s.V(3).Info("security group rule remote address matches 0.0.0.0/0")


Suggested change

s.V(3).Info("security group rule remote address matches 0.0.0.0/0")

s.V(3).Info("security group rule remote address matches %s", infrav1beta2.CIDRBlockAny)

Will update

dharaneeshvrd

Overall SG related changes look's good to me.

dharaneeshvrd · 2024-10-14T05:42:00Z

cloud/scope/vpc_cluster.go

@@ -299,6 +300,36 @@ func (s *VPCClusterScope) GetResourceGroupID() (string, error) {
 	return *resourceGroup.ID, nil
 }

+// GetSecurityGroupID returns the ID of a security group, provided the name.
+func (s *VPCClusterScope) GetSecurityGroupID(name string) (*string, error) {


I don't see this func is getting used. Do you need this func?
If you need for future use, you can replace the checking from status part with getSecurityGroupIDFromStatus() to avoid code duplication.

Yes, I will remove this function.

dharaneeshvrd · 2024-10-14T05:57:28Z

cloud/scope/vpc_cluster.go

+		// Check the Status if an ID is already available for the Security Group.
+		if s.NetworkStatus() != nil && s.NetworkStatus().SecurityGroups != nil {
+			if id, ok := s.NetworkStatus().SecurityGroups[*securityGroup.Name]; ok {
+				securityGroupID = &id.ID
+			}
+		}


can you call getSecurityGroupIDFromStatus() from here to avoid code duplication?

Will update

dharaneeshvrd · 2024-10-14T06:01:19Z

cloud/scope/vpc_cluster.go

+	}
+
+	// Reconcile each Security Groups's Rules.
+	requeue = false


if requeue is true, it would have return from prev block, it won't come here. Why do you want to set requeue to false again?

Will update

dharaneeshvrd · 2024-10-14T06:48:37Z

cloud/scope/vpc_cluster.go

+		// Check the Status if an ID is already available for the Security Group.
+		if s.NetworkStatus() != nil && s.NetworkStatus().SecurityGroups != nil {
+			if id, ok := s.NetworkStatus().SecurityGroups[*securityGroup.Name]; ok {
+				securityGroupID = &id.ID


Once you found securityGroupID, don't you need to go to line 1187?

Yes, will refactor this block

dharaneeshvrd · 2024-10-14T06:50:56Z

cloud/scope/vpc_cluster.go

+		if securityGroupDetails, err := s.VPCClient.GetSecurityGroupByName(*securityGroup.Name); err != nil {
+			// If the Security Group was not found, we expect it doesn't exist yet, otherwise result in an error.
+			if _, ok := err.(*vpc.SecurityGroupByNameNotFound); !ok {
+				return false, fmt.Errorf("error failed lookup of security group by name: %w", err)
+			}
+		} else if securityGroupDetails != nil && securityGroupDetails.ID != nil {
+			securityGroupID = securityGroupDetails.ID
+		}


If you find id via this, then again checking in cloud for its existence in next block is not making sense to me, since it's already returned by cloud API only.

Yes, will refactor this block

dharaneeshvrd · 2024-10-14T07:02:16Z

cloud/scope/vpc_cluster.go

+		s.V(3).Info("Created security group rules", "securityGroupID", securityGroupID, "securityGroupRule", securityGroupRule)
+
+		// Security Group Rules do not have a Status, so we likely don't need to requeue, but for now, will requeue to verify the Security Group Rules
+		return true, nil


What exactly do you want to verify here?
Requeuing for every SG rule creation will overload the controller.
Not needed IMO.

SG Rules are created in bulk, for each new SG.
Verifying the SG Rules for an existing SG require more complex logic to try to prevent creating duplicates (this also applies when verifying all SG Rules were created properly, similar to the other resource types).

I'll drop verification of created SG Rules then.

dharaneeshvrd · 2024-10-14T07:03:27Z

cloud/scope/vpc_cluster.go

+	}
+
+	// Security Group Rules do not have a Status, so we likely don't need to requeue, but for now, will requeue to verify the Security Group Rules
+	return true, nil


Code will never reach here, please handle the else case here.

Currently, the code will be reached after a SG Rule is first created.
If we do not wish to follow up SG Rule creation with verification, then this would be unreachable code, and would be refactored.

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. area/provider/ibmcloud Issues or PRs related to ibmcloud provider labels Oct 3, 2024

k8s-ci-robot requested a review from mkumatag October 3, 2024 21:43

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 3, 2024

k8s-ci-robot requested a review from Prajyot-Parab October 3, 2024 21:43

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 3, 2024

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Oct 3, 2024

cjschaef mentioned this pull request Oct 3, 2024

Extend VPC (x86_64) Infrastructure reconciliation support #1896

Open

cjschaef force-pushed the vpc_reconcile_sgs branch from ece3370 to d3c3f6a Compare October 3, 2024 21:48

cjschaef changed the title ~~WIP: VPC: Extend support for SG's~~ VPC: Extend support for SG's Oct 3, 2024

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 3, 2024

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 7, 2024

Karthik-K-N reviewed Oct 9, 2024

View reviewed changes

VPC: Extend support for SG's

a2baa03

Add support to reconcile SecurityGroups and SecurityGroupRules for VPC extended Infrastructure support. Related: kubernetes-sigs#1896

cjschaef force-pushed the vpc_reconcile_sgs branch from d3c3f6a to a2baa03 Compare October 10, 2024 19:22

Karthik-K-N reviewed Oct 14, 2024

View reviewed changes

dharaneeshvrd reviewed Oct 14, 2024

View reviewed changes

	s.V(3).Info("security group rule remote security group name matches", "securityGroupRuleRemoteSecurityGroupName", sgRule.Name)
	s.V(3).Info("security group rule remote security group name matches", "securityGroupRuleRemoteSecurityGroupName", *sgRule.Name)

	if sgRule.Name != nil && securityGroupRuleRemote.SecurityGroupName == sgRule.Name {
	if sgRule.Name != nil && sgRule.Name == securityGroupRuleRemote.SecurityGroupName{

VPC: Extend support for SG's #1989

Are you sure you want to change the base?

VPC: Extend support for SG's #1989

Conversation

cjschaef commented Oct 3, 2024

k8s-ci-robot commented Oct 3, 2024

k8s-ci-robot commented Oct 3, 2024

netlify bot commented Oct 3, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-cluster-api-ibmcloud ready!

mkumatag commented Oct 7, 2024

Karthik-K-N commented Oct 8, 2024

Karthik-K-N left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjschaef commented Oct 10, 2024

Karthik-K-N left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dharaneeshvrd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented Oct 3, 2024 •

edited

Loading